test: migrate e2e/genutil to systemtest #22325
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughA new test file Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/systemtests/export_test.go (2)
5-13: LGTM: Imports are correctly grouped, but consider removing unused import.The imports are properly organized with standard library imports first, followed by third-party imports. However, the
assertpackage is imported but not used directly in the code.Consider removing the unused import:
- "github.com/stretchr/testify/assert"
39-67: LGTM: Test execution and assertions are comprehensive.The test execution logic is well-structured, covering both error and success cases. The use of a custom assertion function for error cases is a good practice. The verification of exported state, including consensus parameters, is thorough.
Consider adding a comment to explain the
MaxGasconstant, as it's not defined in this file:// MaxGas is defined in system.go and represents the maximum gas limit for transactions require.Equal(t, maxGas, int64(MaxGas))
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- tests/systemtests/export_test.go (1 hunks)
- tests/systemtests/system.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/systemtests/export_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/systemtests/system.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (3)
tests/systemtests/export_test.go (2)
1-3: LGTM: Build tag and package declaration are correct.The build tag
//go:build system_testis properly formatted and appropriate for system tests. The package namesystemtestsfollows Go naming conventions.
15-37: LGTM: Test function structure and setup are well-designed.The
TestExportCmdfunction follows Go naming conventions for tests. The setup, including chain reset and CLI wrapper initialization, is appropriate for system tests. The test cases are well-structured and cover various scenarios for the export command, including error cases and different export options.tests/systemtests/system.go (1)
38-39: New constantMaxGasintroducedThe addition of the
MaxGasconstant with a value of 10,000,000 is a good practice for centralizing the definition of the maximum gas limit. This enhances maintainability by providing a single point of control for this value.
| genesisBz, err = sjson.SetRawBytes(genesisBz, "consensus.params.block.max_gas", []byte(fmt.Sprintf(`"%d"`, MaxGas))) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to set block max gas: %s", err)) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Usage of Gas Limits Detected
Several hardcoded gas limits found alongside MaxGas usage may lead to inconsistencies.
x/simulation/simulate.go:40-45types/tx/types.go:18-19,61-64server/v2/cometbft/abci_test.go:37,335,374,638baseapp/abci_utils.go:262,415
🔗 Analysis chain
Usage of MaxGas constant in SetupChain method
The MaxGas constant is now used to set the maximum gas limit in the genesis block configuration. This change improves code consistency and makes it easier to modify the gas limit across the system if needed.
However, to ensure this change doesn't have unintended consequences, we should verify its usage across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other occurrences of hardcoded gas limits that might need updating
# Search for potential hardcoded gas limits
rg -n '(?i)gas.*limit.*=.*\d{7,}' --type go
# Search for other uses of MaxGas to ensure consistent usage
rg -n 'MaxGas' --type go
Length of output: 5558
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/systemtests/export_test.go (2)
15-44: LGTM: Well-structured test function with comprehensive scenarios.The
TestExportCmd_WithHeightfunction uses table-driven tests to cover various export scenarios effectively. It checks both the initial height and consensus parameters, which is thorough.Consider adding a test case for an invalid height value to ensure proper error handling.
46-77: LGTM: Comprehensive test function for file export scenarios.The
TestExportCmd_WithFileFlagfunction effectively tests both error and success cases for file export. The cleanup of the export file is a good practice.Consider adding a test case for exporting to a file in a non-existent directory to ensure proper error handling in such scenarios.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- tests/e2e/genutil/export_test.go (0 hunks)
- tests/go.mod (1 hunks)
- tests/systemtests/export_test.go (1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/genutil/export_test.go
🧰 Additional context used
📓 Path-based instructions (2)
tests/go.mod (1)
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/systemtests/export_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (6)
tests/systemtests/export_test.go (3)
1-13: LGTM: File header and imports are appropriate.The build tag, package name, and imports are well-structured and relevant for system tests.
1-77: LGTM: Comprehensive test coverage for the export command.The test file provides thorough coverage of the export command functionality, including various scenarios and error cases. The structure is consistent and follows good Go testing practices.
This test file successfully contributes to the PR objective of migrating e2e tests to the systemtest framework.
1-77: LGTM: Code conforms to the Uber Golang style guide.The code follows good Go practices, uses descriptive names, and maintains consistent formatting. No obvious style violations were found.
tests/go.mod (3)
Line range hint
36-39: New dependencies added for testing and module supportSeveral new dependencies have been added, including
cosmossdk.io/core/testing,cosmossdk.io/x/accounts, andcosmossdk.io/x/gov. These additions align with the PR objective of migrating e2e tests to the systemtest framework and potentially introduce new testing capabilities or features.Please ensure that:
- These new dependencies are necessary for the test migration.
- They are compatible with the existing codebase.
- Any new features or capabilities introduced by these dependencies are properly utilized in the migrated tests.
Line range hint
265-293: Updated replace directives for local module testingThe replace directives have been updated to use local versions of various SDK modules. This change ensures that the testing environment uses the most up-to-date versions of these modules, which is crucial for maintaining consistency across interdependent modules during development.
This approach is beneficial for:
- Testing the latest changes in the SDK and related modules.
- Ensuring compatibility between different parts of the Cosmos SDK ecosystem.
- Facilitating easier debugging and development of interdependent modules.
However, please ensure that:
- All referenced local paths are correct and exist in the repository.
- This setup doesn't unintentionally introduce any version conflicts or compatibility issues.
- The CI/CD pipeline is configured to handle these local replacements correctly.
Line range hint
1-293: Overall assessment of go.mod changesThe changes in this
go.modfile primarily focus on updating dependencies and configuring the testing environment to use local versions of SDK modules. These modifications align well with the PR objective of migrating e2e tests to the systemtest framework.Key points:
- New testing-related dependencies have been added, supporting the test migration effort.
- Some existing dependencies have been updated or marked as indirect, potentially optimizing the dependency tree.
- Replace directives have been extensively updated to use local module versions, ensuring tests run against the latest SDK changes.
These changes collectively contribute to a more robust and up-to-date testing environment. However, it's crucial to ensure that these updates don't introduce any unintended side effects or compatibility issues in the broader project context.
| @@ -22,7 +22,7 @@ require ( | |||
| // this version is not used as it is always replaced by the latest Cosmos SDK version | |||
| github.com/cosmos/cosmos-sdk v0.53.0 | |||
| github.com/cosmos/gogoproto v1.7.0 | |||
| github.com/spf13/cobra v1.8.1 | |||
| github.com/spf13/cobra v1.8.1 // indirect | |||
There was a problem hiding this comment.
💡 Codebase verification
Direct usage of github.com/spf13/cobra detected
The github.com/spf13/cobra dependency is still directly used in the codebase. Please review the dependency status to ensure it is correctly marked as indirect.
🔗 Analysis chain
Verify the indirect status of github.com/spf13/cobra
The github.com/spf13/cobra dependency is now marked as indirect. This suggests that it's no longer directly used in the code but is still required by other dependencies. Please verify that this change is intentional and doesn't affect any functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for direct usage of cobra in the codebase
rg -i 'cobra\.' --type go
Length of output: 55686
julienrbrt
added
the
backport/v0.52.x
…osmos-sdk into hieu/systemtest/genutil
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/systemtests/export_test.go (2)
27-35: Add documentation for test casesConsider adding comments explaining each test case's purpose and expected behavior for better maintainability.
Example:
testCases := []struct { name string args []string expZeroHeight bool }{ + // Tests basic export without height parameter - should export at latest height {"should export correct height", []string{"genesis", "export", "--home", sut.nodePath(0)}, false}, + // Tests export with specific height parameter {"should export correct height with --height", []string{"genesis", "export", "--height=5", "--home", sut.nodePath(0), "--log_level=disabled"}, false}, + // Tests export with zero height flag - should reset all heights to 0 {"should export height 0 with --for-zero-height", []string{"genesis", "export", "--for-zero-height=true", "--home", sut.nodePath(0)}, true}, }
55-55: Use constants for test file paths and error messagesConsider extracting the file path and error messages into constants at the package level for better maintainability and reusability.
Example:
+const ( + testExportFileName = "foobar.json" + errNoSuchFile = "no such file or directory" +) + -exportFile := "foobar.json" +exportFile := testExportFileName testCases := []struct { name string args []string expErr bool errMsg string }{ - {"invalid home dir", []string{"genesis", "export", "--home=foo"}, true, "no such file or directory"}, + {"invalid home dir", []string{"genesis", "export", "--home=foo"}, true, errNoSuchFile},Also applies to: 70-71
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- tests/go.mod (1 hunks)
- tests/systemtests/export_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
tests/systemtests/export_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (2)
tests/systemtests/export_test.go (2)
1-14: LGTM: File structure and imports are well-organizedThe build tags, package name, and imports are properly structured following Go conventions.
16-88: 🛠️ Refactor suggestionConsider adding more edge cases to improve test coverage
While the current test coverage is good, consider adding these test cases:
- Export with invalid height parameter
- Export with height greater than chain height
- Export with malformed output file path
- Concurrent export attempts to the same file
Let's check if these cases are covered elsewhere:
| // Wait 10s for producing blocks | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
Replace hard-coded sleep with deterministic block height check
Using time.Sleep for block production can lead to flaky tests. Consider implementing a polling mechanism that checks for the desired block height.
Example implementation:
-// Wait 10s for producing blocks
-time.Sleep(10 * time.Second)
+// Wait for chain to produce some blocks
+require.Eventually(t, func() bool {
+ height, err := sut.QueryCurrentHeight()
+ return err == nil && height >= 5
+}, 30*time.Second, 100*time.Millisecond)Committable suggestion was skipped due to low confidence.
(cherry picked from commit aa4614e) # Conflicts: # tests/go.mod
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #21871
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit